Skip to content

gh-149289: Make lint failures block merge#149290

Closed
JelleZijlstra wants to merge 1 commit intopython:mainfrom
JelleZijlstra:blocklint
Closed

gh-149289: Make lint failures block merge#149290
JelleZijlstra wants to merge 1 commit intopython:mainfrom
JelleZijlstra:blocklint

Conversation

@JelleZijlstra
Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra commented May 2, 2026

@StanFromIreland
Copy link
Copy Markdown
Member

This makes lint run twice on PRs, I think we only need one run.

@hugovk
Copy link
Copy Markdown
Member

hugovk commented May 2, 2026

This means we have two lint runs?

They only take about 25 to 45s each, so not too much considering all the rest, but can we have only one run?

It'll make the list of logs easier to look at (which job should I check?), and during busy sprints, the extra jobs can take up a runner, adding some overhead in waiting for a free runner and so on.

Another option is to mark the existing lint job as required in branch protection rules (I can do it).

@JelleZijlstra
Copy link
Copy Markdown
Member Author

Either works for me! What do you think is best?

@hugovk
Copy link
Copy Markdown
Member

hugovk commented May 2, 2026

I've added it as a required check for all the branches.

@hugovk
Copy link
Copy Markdown
Member

hugovk commented May 2, 2026

Hang on:

image

I'll try the "lint / lint" option instead of "lint".

@hugovk
Copy link
Copy Markdown
Member

hugovk commented May 2, 2026

That's better:

image

Will recheck PRs to the other branches as well.

@hugovk
Copy link
Copy Markdown
Member

hugovk commented May 2, 2026

Hang on again, I shouldn't look at this PR, the "lint" is the one we need! Will flip it back...

@hugovk
Copy link
Copy Markdown
Member

hugovk commented May 2, 2026

Okay, all set, except for 3.10 which doesn't have a lint.yml.

@JelleZijlstra
Copy link
Copy Markdown
Member Author

Thanks!

@zware
Copy link
Copy Markdown
Member

zware commented May 2, 2026

Late to the party, I think this is the change we ideally wanted to make full use of the 'all-green' check. We just needed to completely remove lint.yml rather than gutting it.

@hugovk
Copy link
Copy Markdown
Member

hugovk commented May 3, 2026

We could, but lint.yml is nice and simple, and I would like to avoid putting more complexity in the large build.yml file. Linting and testing are pretty distinct things.

@zware
Copy link
Copy Markdown
Member

zware commented May 3, 2026

The counter-argument is that the all-green check's sole purpose is to avoid as many required checks as we can by bundling them into one. lint should not be a standalone required check, especially since it's something that we should be able to easily bypass if we really need to. If anything at all goes wrong with linting now, we either have to hack around the configuration to make it lie, or bother someone with admin to remove the requirement. I would argue that lint is less important than testing, but we're now in the situation where it's a less involved process to avoid a broken test than broken linting.

Maybe there's a better arrangement of things, but I don't think the simplicity of lint.yml is an argument in favor of keeping three lines out of a configuration file.

@webknjaz
Copy link
Copy Markdown
Member

webknjaz commented May 3, 2026

I also wanted to say that we should have a reusable-lint.yml that would be invoked just once and wired into alls-green. Although, I'd prefer a more unified CI refactor in general that I may bring up in July at the EP sprint since I can't contribute meaningfully right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants